fix(github): number unsafe findings and count the same unit on every surface by aparajon · Pull Request #1152 · block/schemabot

aparajon · 2026-08-26T04:37:32Z

Renders unsafe changes as a flat numbered list — one line per finding — on both the PR comment and the CLI, and makes every heading count the same unit the list shows.

Numbered, flat findings. The heading advertises a count, so the list below is now an ordered list the reader can check it against, and a finding can be referenced by its number ("2 is the blocker"). A table with several joined findings gets one line per finding with the table name repeated, instead of a nested sub-list.
One counting unit everywhere. The PR comment counted individual findings while the CLI counted changes, so the same apply could say "3 unsafe changes" in the terminal and "4" on the PR. Both surfaces now count findings, and the count always equals the number of lines rendered below it.
A reasonless finding still explains itself. When an engine reports an unsafe change without a parseable reason, the PR comment rendered a bare table name while the CLI showed the engine's change type. The PR comment now carries the change type too (3. `audit_log`: drop), falling back to the bare table name only when neither is available.

PR comment — before / after

Before:

⛔ Apply rejected: 3 unsafe changes detected

orders:

Primary key column id has type int

Column created_at uses TIMESTAMP which overflows on 2038-01-19. Consider using DATETIME instead.

users: Column rename detected in table users: email to email_address. …

After:

⛔ Apply rejected: 3 unsafe changes detected

orders: Primary key column id has type int

orders: Column created_at uses TIMESTAMP which overflows on 2038-01-19. Consider using DATETIME instead.

users: Column rename detected in table users: email to email_address. …

CLI — before / after (note the count now matches the list)

Before:

⛔ Apply blocked: 3 unsafe change(s) detected
  • users: DROP COLUMN email
  • orders: DROP TABLE
  • products:
      - MODIFY COLUMN price_cents: INT → SMALLINT (potential data loss)
      - DROP INDEX idx_category

After:

⛔ Apply blocked: 4 unsafe change(s) detected
  1. users: DROP COLUMN email
  2. orders: DROP TABLE
  3. products: MODIFY COLUMN price_cents: INT → SMALLINT (potential data loss)
  4. products: DROP INDEX idx_category

This PR was written by an AI agent (Claude Code, model Claude Fable 5) on behalf of the author.

Copilot

Pull request overview

Updates SchemaBot’s unsafe-change rendering so both GitHub PR comments and the CLI present a flat, numbered list (one line per finding) and ensure any displayed counts match the number of rendered findings.

Changes:

Render unsafe findings as a single ordered list (one numbered line per finding) in PR comments and CLI output.
Make “unsafe change(s) detected” counts consistently reflect the number of rendered findings (not the number of change objects).
Update template previews and tests to assert against the new numbered output format.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
TEMPLATES.md	Updates documentation/examples to show numbered unsafe findings and consistent counts.
pkg/webhook/templates/plan.go	Switches unsafe warning rendering to a flat numbered list and counts findings.
pkg/webhook/templates/apply_commands.go	Uses the shared numbered unsafe-finding rendering for blocked apply comments.
pkg/webhook/plan_test.go	Updates webhook template tests to assert numbered unsafe findings.
pkg/cmd/internal/templates/plan.go	Updates CLI output to count and print unsafe findings as a numbered list.
pkg/cmd/internal/templates/plan_unsafe_test.go	Updates CLI template tests to assert the new numbered output.

Suppressed comments (1)

pkg/webhook/templates/plan.go:1088

*n++ is not valid Go syntax on a pointer dereference. This will fail to compile; increment the dereferenced value with (*n)++ (or *n += 1).

	for _, r := range reasons {
		*n++
		fmt.Fprintf(sb, "%d. %s: %s\n", *n, table, ui.CodeQuoteIdentifiers(r))

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…surface The unsafe-changes headings advertise a count, but the list below was unordered and, on the CLI, counted changes while the PR comment counted findings — the same apply could say 3 in the terminal and 4 on the PR. Both surfaces now render one numbered line per finding (the table name repeats instead of nesting a sub-list), and every heading counts findings, so the count always equals the number of lines below it and a finding can be referenced by its number. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

morgo

🤖 Approved on Morgan's behalf by his AI agent.

Presentation layer only, 37/37 green.

The claim I went after was "count the same unit on every surface" — a fix that corrects one counted heading and leaves a sibling behind is the classic way this goes wrong. writeUnsafeChangesList has three callers, and only WriteUnsafeChangesBlocked was changed. Checked the other two at f5c24ed9: WriteUnsafeChangesWarning heads with "Unsafe Changes Detected:" and WriteUnsafeWarningAllowed with "The following unsafe changes will be applied:" — neither prints a count, so there's no invariant for the now-per-finding list to break. The title holds.

Two things I liked that the description doesn't mention:

It quietly removes an in-place mutation. The old writeUnsafeChangeItem did reasons[i] = ui.CodeQuoteIdentifiers(r) on the slice ui.LintReasons returned. That's fine only as long as LintReasons always hands back a freshly-allocated slice; the moment it caches or returns a sub-slice of something shared, the old code corrupts it for the next caller. Applying the quoting inline at format time makes that class of bug unreachable.
Threading *int through writeUnsafeChangeItem keeps numbering continuous across tables, which is the whole point — per-table restarts would have made "2 is the blocker" ambiguous.

Nits, none blocking:

countUnsafeFindings now exists in both pkg/cmd/internal/templates and pkg/webhook/templates with the same semantics. Two packages, so sharing costs a dependency — probably right to duplicate, just worth a comment on one pointing at the other so they don't drift, since the count/list invariant depends on them agreeing.
The zero-reason fallback still differs by surface: CLI renders table: ChangeType, the PR comment renders a bare table with no explanation at all. Pre-existing, not introduced here, but the comment side is the one an operator reads under pressure.

The PR comment's zero-reason fallback rendered a bare table name while the CLI rendered the engine's change type; thread ChangeType through the webhook unsafe-change data so both surfaces explain the finding. Cross-reference the two countUnsafeFindings helpers so they don't drift. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

aparajon · 2026-08-31T21:27:18Z

🤖 Both nits addressed in a516271:

countUnsafeFindings drift: each copy's doc comment now points at the other and states the invariant (the two must agree so the CLI and PR comment report the same count for the same plan).
Zero-reason fallback: the PR comment now renders the engine's change type like the CLI does — ChangeType was already on apitypes.UnsafeChange at every population site, so it's threaded through UnsafeChangeData and writeUnsafeChangeItem falls back to 3. `audit_log`: drop, with the bare table name only when neither reason nor change type is available.

Reply by Claude (Fable 5).

aparajon mentioned this pull request Aug 26, 2026

feat(lint): add severityglyphs analyzer to keep the severity vocabulary in pkg/glyph #1153

Draft

aparajon force-pushed the armand/glyph-state-cancelled branch from edf7c7c to f63363a Compare August 27, 2026 05:14

aparajon force-pushed the armand/numbered-unsafe-findings branch from a9464f4 to 299ca30 Compare August 27, 2026 05:15

aparajon force-pushed the armand/glyph-state-cancelled branch from f63363a to 479f901 Compare August 28, 2026 08:46

aparajon force-pushed the armand/numbered-unsafe-findings branch from 299ca30 to 4d7f088 Compare August 28, 2026 08:46

aparajon force-pushed the armand/glyph-state-cancelled branch from 479f901 to f9ffbf2 Compare August 28, 2026 09:02

aparajon force-pushed the armand/numbered-unsafe-findings branch from 4d7f088 to b7bd46d Compare August 28, 2026 09:03

aparajon force-pushed the armand/glyph-state-cancelled branch from f9ffbf2 to 99647b1 Compare August 30, 2026 14:00

aparajon force-pushed the armand/numbered-unsafe-findings branch 2 times, most recently from d470d98 to 85e0eca Compare August 30, 2026 21:40

Base automatically changed from armand/glyph-state-cancelled to main August 30, 2026 21:49

aparajon force-pushed the armand/numbered-unsafe-findings branch from 85e0eca to cf1978d Compare August 31, 2026 16:49

Copilot AI lite review requested due to automatic review settings August 31, 2026 16:49

Copilot started reviewing on behalf of aparajon August 31, 2026 16:50 View session

Copilot AI reviewed Aug 31, 2026

View reviewed changes

Comment thread pkg/webhook/templates/plan.go

aparajon force-pushed the armand/numbered-unsafe-findings branch from cf1978d to f5c24ed Compare August 31, 2026 18:24

aparajon marked this pull request as ready for review August 31, 2026 18:36

aparajon requested review from JashLal, Kiran01bm, eeSeeGee, jayjanssen, jemiahw and morgo as code owners August 31, 2026 18:36

morgo approved these changes Aug 31, 2026

View reviewed changes

aparajon merged commit f27ba1b into main Aug 31, 2026
60 of 62 checks passed

aparajon deleted the armand/numbered-unsafe-findings branch August 31, 2026 21:27

Navigation Menu

Uh oh!

fix(github): number unsafe findings and count the same unit on every surface - #1152

fix(github): number unsafe findings and count the same unit on every surface#1152
aparajon merged 2 commits into
mainfrom
armand/numbered-unsafe-findings

aparajon commented Aug 26, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

morgo left a comment

Uh oh!

aparajon commented Aug 31, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

aparajon commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

morgo left a comment

Choose a reason for hiding this comment

Uh oh!

aparajon commented Aug 31, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aparajon commented Aug 26, 2026 •

edited

Loading